Skip to content

Feedback #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: feedback
Choose a base branch
from
Open

Feedback #1

wants to merge 11 commits into from

Conversation

@github-classroom
Copy link

@github-classroom github-classroom bot commented Oct 6, 2025 •
edited
Loading

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to the default branch since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

  • Click the Files changed tab to see all of the changes pushed to the default branch since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
  • Click the Commits tab to see the commits pushed to the default branch. Click a commit to see specific changes.
  • If you turned on autograding, then click the Checks tab to see the results.
  • This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
    For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @mattknatt

@github-actions
Copy link

github-actions bot commented Oct 6, 2025 •
edited
Loading

🤖 AI Feedback

🕒 Posted on 2025-10-20T13:11:04.609Z

Overall Feedback

Looks good! The implementation meets all requirements from the assignment description and passes both BasicTest and EdgeCaseTest. Well done on the comprehensive solution.

Previous Feedback

🕒 Posted on 2025-10-20T12:27:36.762Z

Overall Feedback

Your implementation shows good understanding of the requirements but contains critical logical errors affecting core functionality. Multiple tests fail due to incorrect business logic and insufficient null/edge case handling. Focus on fixing the fundamental business logic errors before addressing polish.

What's Working Well

  • Good use of Java records and streams
  • Appropriate interface implementation
  • Proper factory methods for Category singleton
  • Efficient collection usage (Maps, Streams)
  • Well-structured class hierarchy

Areas for Improvement

Warehouse Class

Issue: expiredProducts() creates inconsistent snapshot
Suggestion: Fix date comparison logic. Current implementation uses isBefore but doesn't properly compare against LocalDate.now():

// Incorrect
return products.values().stream()
    .filter(p -> p instanceof Perishable)
    .filter(p -> ((Perishable) p).isExpired())
    .map(p -> (Perishable) p)

Issue: shippableProducts() returns raw Shippable objects
Suggestion: Use instanceof pattern matching instead of dangerous cast:

public List<Shippable> shippableProducts() {
    return products.values().stream()
        .filter(p -> p instanceof Shippable)
        .map(p -> (Shippable) p) // Replace with instanceof check
        .collect(Collectors.toList());
}

Product Subclasses

Issue: Constructor validation order
Suggestion: Validate all parameters before calling super() in subclasses:

public FoodProduct(UUID id, String name, Category category, BigDecimal price, LocalDate expirationDate, BigDecimal weight) {
    if (price.doubleValue() < 0) throw new IllegalArgumentException("Price cannot be negative.");
    if (weight.doubleValue() < 0) throw new IllegalArgumentException("Weight cannot be negative.");
    super(id, name, category, price); // Only call super after validation
    this.expirationDate = expirationDate;
    this.weight = weight;
}

WarehouseAnalyzer

Issue: Incorrect date range in findProductsExpiringWithinDays()
Suggestion: Fix the date comparison logic - should include end date but not exclude expired items:

public List<Perishable> findProductsExpiringWithinDays(int days) {
    LocalDate today = LocalDate.now();
    LocalDate end = today.plusDays(days);
    return products.stream()
        .filter(p -> p instanceof Perishable perishable)
        .filter(p -> !perishable.expirationDate().isBefore(today))
        .filter(p -> perishable.expirationDate().isBefore(end.plusDays(1))) // FIX: Include end date
        .map(p -> (Perishable) p)
        .collect(Collectors.toList());
}

Issue: getInventoryStatistics() doesn't handle empty collections
Suggestion: Guard against NPE for empty warehouses:

return new InventoryStatistics(
    totalProducts, 
    totalValue,
    items.isEmpty() ? BigDecimal.ZERO : totalValue.divide(BigDecimal.valueOf(items.size()), 2, RoundingMode.HALF_UP),
    expiredCount,
    categoryCount,
    mostExpensiveProduct, // Current implementation returns null for empty list
    cheapestProduct
);

Summary

Fix critical business logic errors in date comparisons and constructor validation first. Multiple core features won't work correctly until these fundamental issues are resolved. The most important takeaway is proper validation sequencing and correct date arithmetic.

Previous Feedback

🕒 Posted on 2025-10-20T12:02:08.075Z

Overall Feedback

All tests pass successfully! The implementation is complete, thorough, and correctly addresses all requirements from both BasicTest and EdgeCaseTest. Excellent work!

Previous Feedback

🕒 Posted on 2025-10-16T06:58:08.753Z

Overall Feedback

The submission demonstrates an excellent implementation of the warehouse kata, especially for the advanced features. The code is well-structured, thoroughly tested, and meets all requirements. The implementation of WarehouseAnalyzer is robust and handles complex business logic effectively.

What's Working Well

  • Test Coverage: All methods in WarehouseAnalyzer pass the corresponding tests in EdgeCaseTest.
  • Correctness: Key algorithms (e.g., weighted average, IQR outlier detection, First-Fit Decreasing) are implemented correctly.
  • State Management: Warehouse behaves as a singleton with proper lifecycle management.
  • Helper Classes: ShippingGroup, InventoryValidation, and InventoryStatistics are well-designed.

Areas for Improvement

1. Weighted Average Precision in calculateWeightedAveragePriceByCategory

Issue: Uses s.weight() (double) for calculations instead of BigDecimal from the product. This may introduce floating-point inaccuracies for high-precision scenarios.
Suggestion: Directly access the product’s BigDecimal weight via casting:

      for (Product p : items) {  
          if (p instanceof Shippable s) {  
// Use internal weight field instead of s.weight()  
BigDecimal actualWeight = s instanceof FoodProduct fp ? fp.internalWeight : ((ElectronicsProduct) s).internalWeight;

2. Division Rounding in getInventoryStatistics

Issue: BigDecimal division uses 2 decimal places, but the test expects 2 decimals for averagePrice.
Suggestion: Explicitly round to 2 decimals with RoundingMode.HALF_UP:

averagePrice = totalValue.divide(BigDecimal.valueOf(totalProducts), 2, RoundingMode.HALF_UP);

3. Null Handling in findProductsInPriceRange

Issue: Test expects null thresholds to throw IllegalArgumentException, but the method currently uses null safely.
Suggestion: Add checks:

if (minPrice == null || maxPrice == null) {  
    throw new IllegalArgumentException("Price cannot be null");  
}

Summary

Well-executed implementation with critical details handled perfectly. The single most important takeaway is verifying edge cases and explicit null/exception handling to ensure robustness.

LGTM

Previous Feedback

🕒 Posted on 2025-10-16T06:35:20.146Z

Overall Feedback

The implementation demonstrates a thorough understanding of the warehouse domain requirements and successfully addresses most advanced scenarios. The solution is well-structured and follows Java conventions with appropriate use of interfaces, polymorphism, and generics.

What's Working Well

  • Excellent handling of the flyweight pattern in Category with proper string normalization and validation
  • Comprehensive implementation of product hierarchy with proper exception handling
  • Robust analytics in WarehouseAnalyzer meeting most edge cases from tests
  • Thoughtful implementation of shipping optimization algorithm

Areas for Improvement

  1. Warehouse changedProducts tracking

    // In Warehouse.java  
public List<Product> getChangedProducts() {  
    return changedProductIds.stream()  
            .map(products::get)  
            .filter(java.util.Objects::nonNull)  
            .toList();  
}

    Issue: Returns changed products that may no longer exist in the warehouse, creating potential stale references.
    Suggestion: Check if product remains in products map before returning.

  2. Potential ClassCastException in shippableProducts

    public List<Shippable> shippableProducts() {  
    return products.values().stream()  
            .filter(p -> p instanceof Shippable)  
            .map(p -> (Shippable) p) // Potential unsafe cast  
            .collect(Collectors.toList());  
}

    Issue: Downcast is unnecessary and could cause exceptions if type checking is incorrect
    Suggestion: Remove cast since filter already guarantees type safety

  3. Concurrency considerations

    • Multiple ConcurrentHashMap operations lack proper synchronization
    • Consider using atomic operations for state transitions
    • Potential race conditions in updateProductPrice and remove methods

Summary

The solution is technically sound with excellent feature coverage. The most important improvement is ensuring proper reference integrity in changed products tracking to prevent stale references while maintaining thread-safe implementations where needed.

Previous Feedback

🕒 Posted on 2025-10-16T05:54:08.832Z

Overall Feedback

The implementation successfully meets all basic requirements and passes all BasicTest cases. However, several EdgeCaseTest failures indicate critical logic gaps in the advanced features. The code quality is generally good but requires specific fixes to satisfy all test cases.

Areas for Improvement

1. Warehouse changedProductIds tracking (Warehouse.java, line 45-48 & line 83-84)
The changedProductIds set isn't properly managed. Products are added to the warehouse without being recorded in changedProductIds, causing getChangedProducts() to return empty results. Update addProduct() to track new additions:

public void addProduct(Product product) {
    if (product == null) throw new IllegalArgumentException("Product cannot be null.");
    if (products.putIfAbsent(product.id(), product) == null) {
        changedProductIds.add(product.id()); // Track new products
    }
}

2. Product constructor accessibility (Product.java, line 18)
The protected constructor should be package-private or private:

class Product(UUID id, String name, Category category, BigDecimal price) { // No public/protected

3. Price validation in setPrice() (Product.java, line 43-45)
Current implementation checks for null but fails negative price checks. Add negative price validation:

public void setPrice(BigDecimal price) {
    if (price == null || price.doubleValue() < 0) { // Handle both null and negatives
        throw new IllegalArgumentException("Price can't be null or negative.");
    }
    this.price = price;
}

4. Weighted average calculation (WarehouseAnalyzer.java, line 106-111)
The weight check incorrectly uses double comparison: w > 0 vs required BigDecimal precision. Use BigDecimal.ZERO.compareTo(w) < 0 for exact comparison.

5. Median calculation in outlier detection (WarehouseAnalyzer.java, line 120-127)
The median() method miscalculates for odd-sized lists. Fix with:

private double median(List<Double> sortedList) {
    int n = sortedList.size();
    return n % 2 == 0
        ? (sortedList.get(n/2 - 1) + sortedList.get(n/2)) / 2.0
        : sortedList.get(n/2);
}

6. ShippingGroup serialization (WarehouseAnalyzer.java, line 26)
Add implements Serializable to ShippingGroup class to pass test annotations:

class ShippingGroup implements Serializable { // Add this

7. Shipping optimization (WarehouseAnalyzer.java, line 168-174)
The algorithm fails edge cases with items that don't fit in existing bins. Improve bin selection logic with:

// In optimizeShippingGroups():
items.sort(Comparator.comparing(Shippable::weight).reversed());
List<ShippingGroup> groups = new ArrayList<>();
for (Shippable item : items) {
    boolean placed = false;
    for (ShippingGroup group : groups) {
        if (group.getTotalWeight() + item.weight() <= maxW) {
            group.add(item);
            placed = true;
            break;
        }
    }
    if (!placed) groups.add(new ShippingGroup(List.of(item)));
}

Summary

Focus on fixing state management issues in Warehouse and precision handling in analytics first. The core logic is sound but requires meticulous attention to edge cases and implementation details specified in the requirements.

Previous Feedback

🕒 Posted on 2025-10-14T17:20:20.291Z

Overall Feedback

The assignment is well-implemented and passes all tests. The source code demonstrates strong understanding of the requirements, with particular attention to edge cases and concurrency. The WarehouseAnalyzer covers all advanced features comprehensively.

What's Working Well

  1. Full Requirements Coverage: All classes, interfaces, and methods from the assignment instructions have been implemented correctly.
  2. Robust Concurrency Handling: Uses ConcurrentHashMap where appropriate for thread safety.
  3. Precise Discount Calculations: The calculateExpirationBasedDiscounts method accurately applies tiered discounts based on expiration proximity.
  4. Accurate Weighted Averages: The calculateWeightedAveragePriceByCategory achieves the required precision (11.43 example).

Areas for Improvement

  1. Standard Deviation Implementation in findPriceOutliers
    Issue: Implementation uses IQR method instead of standard deviation as specified in requirements
    Suggestion: Replace IQR logic with standard deviation calculation. Compute mean and population standard deviation (divide by size in variance calculation). Factor should scale deviations from mean: ```java
    double mean = stats.getMean();
    double stdDev = Math.sqrt(stats.getVariance());
    double lowerBound = mean - factor * stdDev;
    double upperBound = mean + factor * stdDev;

  2. Precision Handling in Weighted Average
    Issue: Mixed double/BigDecimal usage in calculateWeightedAveragePriceByCategory may cause floating-point inaccuracies
    Suggestion: Convert all weight calculations to BigDecimal:

    BigDecimal wBD = BigDecimal.valueOf(w); // Instead of storing in double  
weightedSum = weightedSum.add(p.price().multiply(wBD));  
totalWeight = totalWeight.add(wBD); // Use BigDecimal totalWeight

  3. Single Responsibility Violation
    Issue: WarehouseAnalyzer constructor directly takes Warehouse instead of inverting dependencies
    Suggestion: Create interface WarehouseQueryService for analyzer dependencies to improve testability

    interface WarehouseQueryService {  
    List<Product> getProducts();  
    // Add other query methods  
}  
// Then refactor constructor to accept WarehouseQueryService

Summary

The solution thoroughly implements all requirements but needs a standard deviation fix in outlier detection and improved type safety in weighted calculations. The single most important takeaway is correctly matching the statistical method specified in requirements (standard deviation vs IQR).

Previous Feedback

🕒 Posted on 2025-10-06T06:13:21.549Z

Overall Feedback

Excellent work! Your WarehouseAnalyzer implementation is comprehensive, well-structured, and handles all the advanced requirements from the EdgeCaseTest. The code demonstrates strong understanding of Java collections, BigDecimal operations, date handling, and algorithmic thinking. All the complex business logic for price ranges, expiration discounts, shipping optimization, and statistical analysis is correctly implemented.

The code follows Java conventions, uses appropriate data structures, and handles edge cases properly. The implementation would pass all the EdgeCaseTest requirements.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kappsegla @mattknatt